-
Notifications
You must be signed in to change notification settings - Fork 244
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Testing build fix #2406
Testing build fix #2406
Conversation
/retest |
/retest |
/retest |
/retest |
fca70cc
to
da2f222
Compare
/retest |
/test v4.3-integration-e2e-benchmark |
da2f222
to
377ca01
Compare
pkg/occlient/occlient.go
Outdated
@@ -1682,23 +1683,36 @@ func (c *Client) WaitForBuildToFinish(buildName string) error { | |||
return errors.Wrapf(err, "unable to watch build") | |||
} | |||
defer w.Stop() | |||
timeout := time.After(5 * time.Minute) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be a constant? (high above in occlient.go
)
@@ -1672,7 +1672,8 @@ func (c *Client) StartBuild(name string) (string, error) { | |||
} | |||
|
|||
// WaitForBuildToFinish block and waits for build to finish. Returns error if build failed or was canceled. | |||
func (c *Client) WaitForBuildToFinish(buildName string) error { | |||
func (c *Client) WaitForBuildToFinish(buildName string, stdout io.Writer) error { | |||
following := false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need more description on this, bit confused why we're setting following to false then true when going through the channel loop.
if err != nil { | ||
return err | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of this above looks good!
pkg/occlient/occlient.go
Outdated
@@ -1845,6 +1859,7 @@ func (c *Client) FollowBuildLog(buildName string, stdout io.Writer) error { | |||
} | |||
|
|||
rd, err := c.buildClient.RESTClient().Get(). | |||
Timeout(5*time.Minute). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be a constant
@cdrage Fixed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH, I don't understand the code in this PR. 😞
func (c *Client) WaitForBuildToFinish(buildName string) error { | ||
func (c *Client) WaitForBuildToFinish(buildName string, stdout io.Writer) error { | ||
// following indicates if we have already setup the following logic | ||
following := false | ||
glog.V(4).Infof("Waiting for %s build to finish", buildName) | ||
|
||
w, err := c.buildClient.Builds(c.Namespace).Watch(metav1.ListOptions{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand this is not touched in the proposed PR but can we add a comment that describes what this all about? At least I didn't understand a thing here. 😞
@@ -1682,23 +1685,37 @@ func (c *Client) WaitForBuildToFinish(buildName string) error { | |||
return errors.Wrapf(err, "unable to watch build") | |||
} | |||
defer w.Stop() | |||
timeout := time.After(OcBuildTimeout) | |||
for { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't understand what's going on in this for
loop either. 😞
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can help here a bit. So here first thing to observe is the select
statement on a quick TLDR: select statements are like channel collectors - listen on multiple channels at once, more here https://tour.golang.org/concurrency/5.
now the Build().Watch()
returns a channel which returns messages on as per the pod status. Now the change that has been done here is
we wait for the pod to start before we start following the logs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as for the need for the following
, The w.ResultChan()
can send multiple messages of type BuildPhaseRunning
. But we dont have to follow logs multiple times, just the first time should be good enough. Hence this flag.
Think of it as a singleton
@dharmit Updated with more comments |
/retest |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: girishramnani The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/test all |
/lgtm |
What kind of PR is this?
/kind flake
What does does this PR do / why we need it:
Which issue(s) this PR fixes:
Fixes #1981
How to test changes / Special notes to the reviewer: